fix(ci): stop the benchmark suite starving CI and publishing partial numbers - #350

…numbers

Three open issues on 2.4.0 turned out to be one lane: nothing rationed the
most expensive workflow in the repository.

Runs accumulated. benchmarks.yml declared no concurrency group, so five
pushes over one review loop created five uncancelled eight-shard runs and
left CI and Coverage queued for ~50 minutes behind numbers nobody would
read. The workflow now supersedes its own in-flight run, keyed on the PR
number; the main path is keyed on the commit SHA instead, so every commit
is its own group, cancel-in-progress can never discard one, and the
published series keeps every point. Closes #319.

Runs overlapped. With three branches in flight every shard measured
1.75-2.0x its baseline and one hit the 120-minute cap - on two pull
requests whose diffs were XML doc comments only. The quiet failure is the
worse one: a shard that finishes under uneven contention still publishes
its skewed delta. scripts/benchmark_relevant_changes.js now gates the PR
path and does not run the suite when the diff cannot move a number. It is
one-directional by construction (only documentation, the three projects
Celerity.Benchmarks.csproj does not reference, and .cs files whose text is
unchanged once comments are stripped can be skipped) and never applies to
main, so a gate mistake costs a missing PR comment rather than an unseen
regression. It correctly skips both pull requests the issue names and runs
on every code change it was tested against.

The issue's own first choice - a global serialize-everything concurrency
group - was deliberately not taken: GitHub queues at most one pending run
per group and cancels the older pending one, so serializing would silently
drop runs. What ships is its option 2, which it rated cheapest and most
obviously correct. Closes #335.

The two sides of the A/B packed from different class lists. Greedy
bin-packing is a function of the whole list and the PR head has a class
main does not, so shard i was not the same slice on both sides and could
pair a light head slice with a heavy base one. The base now replays the
class list the head resolved, which makes it a subset of the head by
construction: the pair is bounded by twice the head slice, the quantity
the packer already balances. Option 4 of that issue ships alongside - a
report missing a shard says so above the fold, since a partial comparison
previously read exactly like a complete one. Closes #300.

The comment-stripping rests on a real C# scanner rather than a //-prefix
test, because // occurs inside literals and the verbatim / interpolated /
raw forms desynchronise a guess; a --self-test pins it in a new
benchmark-gate job. --shard-dry-run resolves a shard's class list without
measuring, so the packing is inspectable without a multi-hour run.

Both from the Copilot review, both correct.

The unbenchmarked-project list named only three of the six projects the
benchmark process cannot load, so a change confined to Celerity.Ring.Tests,
Celerity.Sentinel.Tests or Celerity.Cardinality.Tests still bought a full
sharded run. Rather than append three more strings to a list that had
already drifted once, the rule is now stated as the reachable set
(BENCHMARKED_PROJECTS, following Celerity.Benchmarks.csproj transitively)
plus a convention for the rest, and --self-test refuses to pass while any
project directory under src/ is classified by neither. A project that fits
no rule is still treated as significant, so the drift guard fails loudly
without ever making the gate less safe.

normalize() trimmed every line after comment-stripping, which is right for
code and wrong inside a multi-line verbatim or raw literal, where the
indentation and the blank lines are part of the string value and reach the
IL. Reindenting such a literal therefore read as "comments only". A newline
inside a string frame is now emitted as a sentinel instead, so each literal
occupies one logical line and the per-line trim can only touch the code
around it. Pinned both ways: reindenting a literal must not normalize
identically, and a comment edit beside one must.

Also batched from my own re-read: the relevance-gate job no longer does a
full-history checkout on the push path, where it answers without consulting
the repository at all and the clone was pure latency ahead of the matrix;
and the gate's git invocations set core.quotePath=false so a path with
non-ASCII characters is compared exactly rather than taking the fail-safe
run path.

Historical verdicts are unchanged: the two documentation-only pull requests
still skip and every code change still runs.

…entries

Both from the second Copilot review, which reported "no new comments" and
then carried two in a suppressed block.

--shard-classes read the roster into a HashSet and filtered the suite by it,
so the base ran the right classes in the wrong order: declaration order
rather than the packed order the head wrote. The set is what bounds the job,
so this was not the timeout bug, but running the two sides in different
orders reintroduces in miniature the systematic head/base difference the
whole handoff exists to remove. The roster is now walked in file order,
de-duplicated on the way, and a name this side does not have is reported
where it appears rather than in a separate sorted pass. Verified: head shard
1 and its replay now resolve to the identical sequence, where before the
replay came back reordered.

The changelog entries ran to the incident narrative that CONTRIBUTING.md
says belongs in the PR body — a release-safety rule here, not only style,
since the release workflow extracts a whole version section verbatim as the
GitHub release body. Five entries condensed to what observably changed and
why it matters; the narrative is already in the PR description.

… measurement

The first full run of this workflow found a real defect in it, which no
amount of local testing could have: every shard was cancelled at the
120-minute cap.

The base step builds a worktree at the `main` tip and runs THAT code, so it
only understands flags already on `main`. Switching the base from --shard to
--shard-classes meant a base that predates the flag matched nothing, fell
through to the whole suite, and ran until the job timeout: shard 0's head
measured 3 classes and 97 benchmarks in 66 minutes, then the base logged
"Found 12 benchmarks" nine times over in 53 minutes before being killed. This
is the same transitional trap the comment I replaced had been warning about
for --shard itself. Both selectors are now passed, so the base packs its own
slice until this merges and replays the head's afterwards, and the constraint
is written down where the next selector will be added.

The run also measured what the cap should be. The eight head slices took
45.8, 48.7, 55.4, 55.5, 56.8, 58.2, 63.9 and 67.3 minutes; the base replays
the head's slice, so a job costs about twice its head slice, and shards 0 and
2 exceed 120 minutes on their own. The cap was set when the suite was smaller
and Sorting, SortedSpan and SegmentTree have each added classes since, so
120 -> 180 is sizing the budget to the bound rather than buying room for an
imbalance -- the imbalance is what the roster handoff fixes, and it had to
exist first for this number to mean anything.

Widening the matrix was the alternative and is rejected in the comment: the
81-case StringHasherBenchmark is one class and sharding is by class, so it
floors the heaviest slice however many shards there are, and more shards
means more concurrent runners -- the contention this change exists to reduce.
If slices reach 90 minutes the fix is to split that class, not to raise this
again.

Verified locally that the new code still prefers the roster when both flags
are present: with --shard 5 and shard 0's roster it resolves to shard 0's
three classes in order, where --shard 5 alone packs a different seven.